feat(metrics): Add metrics section to issue details #103092
Conversation
github-actions
bot
added
the
Scope: Frontend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
static/app/views/explore/metrics/metricInfoTabs/metricsSamplesTableRow.tsx
Show resolved
Hide resolved
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| traceIds={freeze?.traceIds ?? []} | ||
| tracePeriod={freeze?.tracePeriod} | ||
| > | ||
| <QueryParamsContextProvider | ||
| <QueryContextProvider | ||
| queryParams={queryParams} | ||
| setQueryParams={setWritableQueryParams} | ||
| isUsingDefaultFields | ||
| shouldManageFields={false} | ||
| > | ||
| {children} | ||
| </QueryParamsContextProvider> | ||
| </QueryContextProvider> |
There was a problem hiding this comment.
Bug: QueryContextProvider is incorrectly instantiated with MetricsStateQueryParamsProvider due to incompatible prop signatures, causing a TypeScript compilation error.
Severity: CRITICAL | Confidence: 1.00
🔍 Detailed Analysis
The QueryContextProvider is incorrectly used with MetricsStateQueryParamsProvider when isStateBased=true. This leads to a TypeScript compilation error because MetricsStateQueryParamsProvider does not accept queryParams, setQueryParams, isUsingDefaultFields, and shouldManageFields props, which are expected by QueryContextProvider. This fundamental type mismatch prevents the code from compiling.
💡 Suggested Fix
Either MetricsStateQueryParamsProvider needs to accept frozenParams to receive parent state, or the caller must stop passing incompatible props like queryParams and setQueryParams when isStateBased is true.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: static/app/views/explore/metrics/metricsQueryParams.tsx#L82-L99
Potential issue: The `QueryContextProvider` is incorrectly used with
`MetricsStateQueryParamsProvider` when `isStateBased=true`. This leads to a TypeScript
compilation error because `MetricsStateQueryParamsProvider` does not accept
`queryParams`, `setQueryParams`, `isUsingDefaultFields`, and `shouldManageFields` props,
which are expected by `QueryContextProvider`. This fundamental type mismatch prevents
the code from compiling.
Did we get this right? 👍 / 👎 to inform future reviews.
| export function defaultSortBys(fields: string[]): Sort[] { | ||
| // TODO: Handle compound sort-by. | ||
| return [{field: fields[0]!, kind: 'desc'}]; |
There was a problem hiding this comment.
Bug: defaultSortBys in metrics module sorts by id instead of prioritizing timestamp, deviating from established codebase patterns.
Severity: HIGH | Confidence: 0.90
🔍 Detailed Analysis
The defaultSortBys function in the new metrics module sorts by fields[0] (which is 'id' when called with ['id', 'timestamp']), instead of prioritizing 'timestamp'. This deviates from the established pattern in logsQueryParams.tsx, spansQueryParams.tsx, and the shared sortBys.tsx, all of which prioritize 'timestamp' for sorting. Consequently, metrics samples will sort by 'id' descending, altering user experience and violating a standard UX pattern.
💡 Suggested Fix
Modify the defaultSortBys function in the metrics module to prioritize 'timestamp' when available, consistent with other modules and shared utilities.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: static/app/views/explore/metrics/metricQuery.tsx#L135-L137
Potential issue: The `defaultSortBys` function in the new metrics module sorts by
`fields[0]` (which is 'id' when called with `['id', 'timestamp']`), instead of
prioritizing 'timestamp'. This deviates from the established pattern in
`logsQueryParams.tsx`, `spansQueryParams.tsx`, and the shared `sortBys.tsx`, all of
which prioritize 'timestamp' for sorting. Consequently, metrics samples will sort by
'id' descending, altering user experience and violating a standard UX pattern.
Did we get this right? 👍 / 👎 to inform future reviews.
❌ 2 Tests Failed:
View the top 2 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
…how-on-issues-view
| > | ||
| {children} | ||
| </QueryParamsContextProvider> | ||
| </QueryContextProvider> |
There was a problem hiding this comment.
Bug: Provider Ignores Intended State Control
When isStateBased is true, the code attempts to pass props like queryParams, setQueryParams, isUsingDefaultFields, and shouldManageFields to MetricsStateQueryParamsProvider, but this provider only accepts children and frozenParams. This mismatch will cause MetricsStateQueryParamsProvider to ignore these props and use its own internal state management instead, breaking the intended behavior where queryParams and setQueryParams control the provider's state.
There was a problem hiding this comment.
Bug: Promises: Stuck in Limbo
The getTagValues prop creates a Promise that never resolves. The executor function () => [] returns an array but doesn't call resolve or reject, causing the promise to hang indefinitely. This should be Promise.resolve([]) or new Promise((resolve) => resolve([])).
static/app/views/performance/newTraceDetails/traceMetrics.tsx#L107-L108
sentry/static/app/views/performance/newTraceDetails/traceMetrics.tsx
Lines 107 to 108 in 4529f98
| <SearchQueryBuilder | ||
| placeholder={t('Search metrics for this trace')} | ||
| filterKeys={{}} | ||
| getTagValues={() => new Promise<string[]>(() => [])} |
There was a problem hiding this comment.
Bug: Unresolved Promises Cause Indefinite Hangs
The getTagValues prop creates a Promise that never resolves. The executor function () => [] returns an array but doesn't call resolve or reject, causing the promise to hang indefinitely. This should be Promise.resolve([]) or new Promise((resolve) => resolve([])).
Adds a metric sections to issue details, only shown when there are >0 metrics and there is a trace. Also in this PR; modifying the providers to allow state based for it being embedded and searchable inside issue details with spamming url params. Closes LOGS-369
Adds a metric sections to issue details, only shown when there are >0 metrics and there is a trace. Also in this PR; modifying the providers to allow state based for it being embedded and searchable inside issue details with spamming url params. Closes LOGS-369
Adds a metric sections to issue details, only shown when there are >0 metrics and there is a trace.
Also in this PR; modifying the providers to allow state based for it being embedded and searchable inside issue details with spamming url params.
Closes LOGS-369